Skip to content

fix(Typeahead,Tokenizer): the busy indicator is a Spinner in the field's end lane, not a clock on top of the clear button - #5555

Open
freddymeta wants to merge 4 commits into
mainfrom
fix/typeahead-busy-indicator
Open

fix(Typeahead,Tokenizer): the busy indicator is a Spinner in the field's end lane, not a clock on top of the clear button#5555
freddymeta wants to merge 4 commits into
mainfrom
fix/typeahead-busy-indicator

Conversation

@freddymeta

@freddymeta freddymeta commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

No longer stacked#5385 landed, so this is replanted on main and reviewable on its own. Closes #5554.

What was wrong

Three defects in one block, all on main:

  1. The busy indicator is a static clock. BaseTypeahead.tsx rendered <Icon icon="clock" size="sm" color="secondary"> — byte-identical to TimeInput.tsx, and in core clock otherwise means time. Every sibling input paints busy with <Spinner size="sm" />. Nothing spun during a search: getAnimations({subtree: true}) finds nothing running on it.
  2. It landed on top of the clear button. The indicator was an in-flow flex item and the input before it is flex: 1, so it was pushed to the row's inline end — which is where each wrapper independently parks an absolutely-positioned clear affordance. 17×20px at Typeahead md (13 of the 16px glyph covered) and 19×20px in Tokenizer, where the indicator painted over the button and left part of the ✕ unclickable.
  3. The combobox never got aria-busy, unlike TextInput.

The change

The engine reports the busy state rather than painting it, and each field renders <Spinner size="sm" /> in the one lane it already owns at its inline end — Tokenizer's endSection, which already holds endContent and the clear button, and Typeahead's clear-button box, widened into an endLane flex row. aria-busy goes on the input.

A caller using BaseTypeahead directly is unaffected: it still renders its own visible, named "Loading" status, as a Spinner now rather than the clock, so the fix reaches those callers too. Passing the callback is what hands the indicator over, so a field never paints two.

The input reserves the lane. The lane is absolutely positioned — both wrappers are flexWrap: 'wrap', so an in-flow sibling gets pushed onto a second row by a token — and an out-of-flow box reserves nothing, so at a narrow width the live query ran underneath it. useEndLaneReserve measures the rendered lane and returns the padding the input needs. Measured rather than assumed: the lane holds a clear button that comes and goes with the value, an indicator that comes and goes with the search, and, in Tokenizer, arbitrary endContent. There is no CSS that does this — the input cannot see a sibling's width, and a custom property set on the lane cannot travel sideways to it.

Measured in Chromium, at 280px

Overlap of the input's content box (where text and the caret may go) with each lane control, across the six states, against a build of current main:

state main here
Typeahead, value settled clear 17px 0
Typeahead, value + search in flight 0 0
Typeahead, value idle 0 0
Tokenizer, value idle clear 25px 0
Tokenizer, value settled clear 25px 0
Tokenizer, value + search in flight 0 0

The two in-flight rows were already 0 on main — but only because the in-flow indicator reserved its own width. Moving it into the lane is what would have taken that away, which is the regression @cixzhang caught; the reserve is what holds them at 0. The other rows are the pre-existing case of the same bug, with no spinner in it at all — I had filed that away as separate and it belongs here, since it is the same missing reserve.

Indicator, same probe: Icon/0 animations on main → Spinner/1 animation here, both fields. aria-busy absent on main → true on both.

Review changes since the last revision

  • The input reserves the rendered lane width — the table above.
  • The default status is preserved for direct callers. BaseTypeaheadProps is re-exported from the package entry point, so the base has callers this repo cannot see, and they painted no indicator of their own. It renders whenever no wrapper has taken it over.
  • The transport is internal: __onLoadingChange, marked @internal, following __queryEntries from feat(core): minQueryLength — hold the typeahead search until the query is long enough #5385 and DefinedTheme.__inputTokens.
  • No Effect. The report goes out at the call site through a ref, so the field's state change batches into the commit React was already doing instead of forcing a second one — the doubled wrapper commits are gone. It is edge-triggered as well, so the redundant clear on every keystroke below the query threshold reports nothing at all.
  • A changeset, which was missing.

Coverage

There was none — no story rendered either component loading, and neither test file exercised isLoading, which is how a clock survived in the busy slot. Adds a Loading story to each (async source, 1.2s), the Tokenizer one with hasClear and endContent so all three lane occupants are present at once, plus unit tests for the three contracts jsdom can hold: the default status for a direct caller, the handover (and that the base then renders none), and the edge-triggered reporting.

Test plan

  • pnpm build, pnpm lint:strict (0 errors; the same 80 warnings main reports), pnpm check:repo, pnpm -F @astryxdesign/core typecheck — all clean.
  • pnpm exec vitest run packages/core/src/Typeahead packages/core/src/Tokenizer: 132 tests pass.

@vercel

vercel Bot commented Aug 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 28, 2026 8:16am

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 26, 2026
@freddymeta

Copy link
Copy Markdown
Contributor Author

Negative control, run after the fact rather than asserted — the same two stories and the same probe, against main's components:

main this PR
Typeahead indicator Icon, 0 animations Spinner, 1 animation
Typeahead overlap 17 px (303…327 over clear 310…330) 0 px (292…306, clear 310…330)
Tokenizer indicator Icon, 0 animations Spinner, 1 animation
Tokenizer overlap 19 px (388…412 over clear 387…407) 0 px (284…298, clear 387…407)
aria-busy absent on both true on both

0 animations is the "static clock" claim measured rather than eyeballed: getAnimations({subtree: true}) finds nothing running on the indicator on main.

Gates: 12285 tests pass, lint:strict 0 errors, typecheck clean. The one failing test is story-tree.test.tsCore/Icon/Size Theming, which is red on main from #5465 and only surfaces once apps/storybook/dist exists locally.

@freddymeta
freddymeta marked this pull request as ready for review August 26, 2026 22:10

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the Spinner and aria-busy are the right direction, but moving loading out of BaseTypeahead leaves three regressions.

At 280px the absolute lane covers the live query: Typeahead’s Spinner overlaps 14×14px of the input and clear overlaps 17×20px (both zero on the parent); Tokenizer has the same Spinner/input overlap. The caret and trailing characters render under the controls while search runs. Could the input reserve the rendered lane width?

Parent Head Settled control
Parent busy Head busy Head settled

BaseTypeahead is released. Direct callers previously got a visible, named “Loading” status; this head removes it and adds an undocumented public callback, so existing callers silently lose that feedback. Its Effect also doubles wrapper commits at search start and settlement. Could we preserve the default status, keep wrapper transport internal, report the transition without an Effect, and add a changeset?

[Reviewed by Robohands]

@freddymeta
freddymeta changed the base branch from feat/typeahead-min-query-length to main August 27, 2026 03:02
@github-actions
github-actions Bot requested a review from cixzhang August 27, 2026 03:20
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review labels Aug 27, 2026
@freddymeta
freddymeta force-pushed the fix/typeahead-busy-indicator branch from 1eb979d to 404864d Compare August 27, 2026 03:24
@freddymeta

Copy link
Copy Markdown
Contributor Author

All four are in, and you were right about the regression — thanks for measuring it.

The overlap. My "pre-existing, filing separately" note was wrong in the state you tested. The in-flow indicator I removed was reserving its own width, so moving it into the lane is what took the reserve away while a search is out. The lane is out of flow and reserves nothing, and no CSS fixes that from the input's side: it cannot see a sibling's width, and a custom property set on the lane cannot travel sideways to it. So useEndLaneReserve measures the rendered lane and returns the padding the input needs — measured rather than assumed, because the lane holds a clear button that comes and goes with the value, an indicator that comes and goes with the search, and, in Tokenizer, arbitrary endContent.

That also folds in the case I had punted, since it is the same missing reserve without a spinner in it. At 280px, overlap of the input's content box with each lane control, against a build of current main:

state main here
Typeahead, value settled clear 17px 0
Typeahead, value + search in flight 0 0
Typeahead, value idle 0 0
Tokenizer, value idle clear 25px 0
Tokenizer, value settled clear 25px 0
Tokenizer, value + search in flight 0 0

The released base. BaseTypeahead renders its own visible, named status again, whenever no wrapper has taken the indicator over — so a direct caller loses nothing, and gains the Spinner in place of the clock. Passing the callback is what hands it over, so a field never paints two. The transport is __onLoadingChange, @internal, following __queryEntries from #5385.

The Effect is gone. The report goes out at the call site through a ref, so the wrapper's setState batches into the commit React was already doing rather than forcing a second one. It is edge-triggered too, which fixes something I had not noticed: every keystroke below minQueryLength clears the flag, so the Effect version handed the wrapper a false per character while the user typed. Now those report nothing at all. Three unit tests cover the default status, the handover, and the report count.

Changeset added.

One process note: #5385 landed while I was working on this, so the branch is replanted on main and is no longer stacked — the diff is this change only.

@freddymeta

Copy link
Copy Markdown
Contributor Author

Audited the change rather than just re-running my own tests. Two things fixed, three verified, one worth your judgement.

Fixed

  • The latest-callback ref was assigned during render. The repo splits on exactly this case: plain values go during render (snapPointsRef in BottomSheetPanel), but the callback refs beside them — onMotionStartRef, onMotionCompleteRef — sync in a layout effect, so a render React discards cannot leave the ref pointing at the abandoned pass's callback. Now matches. It writes a ref and nothing else, so no wrapper re-renders for it and the doubled commits stay gone.
  • The ResizeObserver fallback read contentRect (content box) where the primary path reads borderBoxSize. Equal today — the lane has no padding or border — and silently short the moment one is added. Reads offsetWidth now.

Verified

  • Every size, both directions, under StrictMode: sm/md/lg × LTR/RTL × busy/settled, both components — spinner overlap 0 and clear overlap 0 in all of them, and the padding lands on the inline-end side only in RTL (it is logical, so it mirrors). Worth noting lg was already inheriting the md lane inset before this change; my reserve uses the same ternary as the lane's positioning, so the two agree at every size.
  • No feedback loop and no console noise. The reserve changes the input's padding, which could in principle resize the lane and re-fire the observer — it does not, because the lane is out of flow and sized by its own content. Zero console errors or warnings across the whole matrix, StrictMode included, so no ResizeObserver loop completed either.
  • The reserve does not squeeze the text area. Fields from 360px down to 140px with all three lane occupants present: no row loses its content box, and the input is in fact wider than on main at every width (342px vs 314px at 360px), because removing the in-flow indicator gives more room back than the reserve takes.
  • Full packages/core suite: 283 files / 8024 tests pass. lint:strict 0 errors (the same 80 warnings main reports), check:repo clean including check:sync for the new file, typecheck clean for core and Storybook.
  • Both new stories render in a built Storybook: aria-busy="true", the indicator is an SVG with 1 running animation, overlap 0, no console errors.

Your call, not a defect

Core/Tokenizer → Loading reserves 131px, because the lane genuinely is 123px there — spinner + "1 selected" end content + clear button. The field keeps a 145px text area at its 400px story width, so it is correct rather than miscalculated, but a wide endContent does eat the input by exactly its own width. That is inherent to reserving honestly; on main the text simply ran underneath it. If you would rather the input scrolled under a wide lane instead of shrinking, that is a different trade and I would take it as a follow-up.

One correction to my earlier comment: I claimed Selector, MultiSelector, CommandPalette and DateTimeInput were direct BaseTypeahead callers. They are not — those matches were comments referencing it. The real direct callers are Typeahead, Tokenizer, and the CLI showcase block blocks/components/BaseTypeahead/BaseTypeaheadCustomSearch.tsx, which is the in-repo example of the case you flagged and now renders the preserved Spinner status.

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the prior overlap, direct-Base, internal-transport, and changeset asks are fixed. The render-cost ask is still open: same-worktree A/B takes a 20-token search from 20 to 120 token renders, and disabling only the reserve returns it to 60. Could we keep the zero-overlap result without adding full-field commits at search start and settlement?

Prior head Current head
Prior head Current head

[Reviewed by Robohands]

return;
}

const observer = new ResizeObserver(entries => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the existing shared observer instead of allocating one per field lane.

freddymeta and others added 3 commits August 27, 2026 17:12
…d's end lane, and the input reserves it

Three defects in one block. The indicator a search painted was
`<Icon icon="clock">` — a static glyph, byte-identical to TimeInput's, in a
family where every other input paints busy with a Spinner and where `clock`
otherwise means *time*. It was an in-flow item at the row's inline end, which
is where each field independently parks its clear button, so the two landed on
each other: 17x20px of overlap in Typeahead, 19x20px in Tokenizer, the latter
leaving part of the clear glyph unclickable. And the combobox never carried
`aria-busy`.

The engine reports the busy state instead of painting it, and each field
paints it in the one inline-end lane it already owns beside its clear button
and end content. A direct `BaseTypeahead` caller keeps the visible, named
status it has always had — as a Spinner now, so the fix reaches those callers
too — and passing the callback is what hands the indicator over, so a field
never renders two.

The lane is out of flow (these wrappers wrap, and an in-flow sibling gets
pushed onto a second row by a token), so it reserved nothing and the query ran
underneath it at a narrow width. `useEndLaneReserve` measures the rendered
lane and returns the padding the input needs. Measured rather than assumed,
because what the lane holds varies with the field's state and, in Tokenizer,
includes arbitrary `endContent`.

At 280px, all six states measured, overlap of the input's content box with
the lane's controls:

| | main | here |
|---|---|---|
| Typeahead, value settled | clear 17px | 0 |
| Typeahead, value + search in flight | 0 | 0 |
| Tokenizer, value idle / settled | clear 25px | 0 |
| Tokenizer, value + search in flight | 0 | 0 |

The two rows that were already 0 were only 0 because the in-flow indicator
reserved its own width; moving it out is what would have regressed them, and
the reserve is what holds them. The other two are the pre-existing case of the
same bug, with no spinner in it at all.

Reporting goes out at the call site through a ref rather than from an Effect,
so the field's state change batches into the commit React was already doing
instead of forcing a second one, and it is edge-triggered, so the redundant
clear on every keystroke below the query threshold reports nothing.

Stacked on #5385 until it landed; replanted on main now that it has.
… during render

Two audit findings, neither user-visible.

The latest-callback ref was assigned during render. The repo's own convention
splits on exactly this: plain values are assigned during render
(`snapPointsRef` in BottomSheetPanel), but the latest-callback refs beside
them — `onMotionStartRef`, `onMotionCompleteRef` — are synced in a layout
effect, because a render React discards must not leave the ref pointing at a
callback from the abandoned pass. This effect writes a ref and nothing else,
so it commits nothing and no wrapper re-renders for it; the doubled commits
the review asked about stay gone.

And the ResizeObserver's fallback for a browser with no `borderBoxSize` read
`contentRect`, which is the content box — equal today, since the lane has no
padding or border, and silently short the moment one is added. `offsetWidth`
on the observed element is border-box, like the primary path.
The reserve fixed the overlap and paid for it in commits: a 20-token
search went from 20 renders to 120, and the reserve alone accounted for
the second half. `useEndLaneReserve` held the measured width in state,
so the lane changing size — which it does exactly twice a search, as the
spinner arrives and as it leaves — re-rendered the whole field to carry
a number no JavaScript ever reads.

It reaches CSS as a custom property written straight to the field
wrapper and inherited by the input, so the padding follows the lane
without React seeing the value at all. The rule is static now: one
class, `calc(inset + var(--_astryx-end-lane-width, 0px))`, generated
once instead of regenerated per width.

Measured, same test either way — `Profiler` around the field, one search
start to settle:

    state-held reserve   2 commits at search start   (fails)
    custom property      1 commit                    (passes)

which is the doubling, and it holds for the settle edge too. jsdom
reports every width as 0, so a state-held reserve never re-renders
there and the regression is invisible; the test stubs a ResizeObserver
that reports a width, which is the smallest thing that makes it
reproducible in CI.

The observer is shared as well — `observeResize`, the same singleton
`useTruncation` uses. Three fields on a page created three observers
before and create one now, so the browser dispatches one callback a
frame rather than N.

Verified in Chromium at 280px that the accepted fix still holds: while
busy, the input's content box ends at 268 and the lane starts at 276 —
zero overlap with the caret, and the reserve is released when the lane
goes.
@freddymeta
freddymeta force-pushed the fix/typeahead-busy-indicator branch from 1c34260 to b3ea3b1 Compare August 28, 2026 00:29
@github-actions
github-actions Bot requested a review from cixzhang August 28, 2026 00:29
@freddymeta

Copy link
Copy Markdown
Contributor Author

Render cost fixed — the reserve now costs zero commits of its own (b3ea3b1b5), and it is rebased onto main.

Root cause. useEndLaneReserve held the measured width in React state. The lane changes size exactly twice a search — spinner in, spinner out — so each transition re-rendered the whole field to carry a number no JavaScript ever reads. That is the doubling you measured.

Fix. The measurement reaches CSS as a custom property written straight to the field wrapper and inherited by the input. The rule is static now:

paddingInlineEnd: calc(<lane inset> + var(--_astryx-end-lane-width, 0px))

One class, generated once, instead of a new one per width. React never sees the value.

Measured, same test against both implementationsProfiler around the field, one search from start to settle:

commits at search start total
state-held reserve 2 4
custom property 1 2

The remaining two are the ones the search itself owes: the spinner arriving and the spinner leaving.

Worth flagging how that test is built, because the obvious version does not work: jsdom reports every width as 0, so a state-held reserve never re-renders there and the regression is invisible. The test stubs a ResizeObserver that reports a width on observe — the smallest thing that makes it reproducible in CI. It is written to be implementation-neutral (it waits on no DOM signal either implementation owns), so it genuinely fails on the old code rather than erroring on it: I ran it both ways to confirm.

And the observer is shared. It now goes through observeResize — the same singleton useTruncation uses — rather than constructing one per lane. Three fields on a page created three observers before and create one now, so the browser dispatches one callback a frame rather than N. There is a test for that too: instances <= 1 across three mounted fields, which fails at 3 on the old code.

The accepted fix still holds. Re-measured in Chromium at 280px while busy:

input content box ends   268
lane starts              276     → 0px overlap with the caret
--_astryx-end-lane-width 14px
padding-inline-end       19px    (5px inset + 14px lane)

and on settle the property is removed, so the input takes the room back rather than keeping a stale reserve.

Changeset updated with both the property and the shared observer.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Field (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 809 -
Complexity N/A Very High (48) -
Tokenizer (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 703 -
Complexity N/A Very High (107) -
Typeahead (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 1272 -
Complexity N/A Very High (185) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 serious.

Tokenizer - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/22 stories · Learn more
    • WCAG: 1.4.3 (Level AA)

Visual Regression

634 added · 0 removed. View the report

To accept these exact frames: /accept-visual 33154460467/1 <reason>

Added — After
Added — After visual regression frame

Added — After
Added — After visual regression frame

Added — After
Added — After visual regression frame


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

@freddymeta

Copy link
Copy Markdown
Contributor Author

/accept-visual

The report lists 634 added, zero changed — new baselines for stories that had none, and not a single existing shot moved. That is the expected result: this round is a render-cost refactor (the lane width moved from React state to a CSS custom property), and the computed padding is identical either way. Verified in Chromium at 280px — content box ends 268, lane starts 276, same as before.

@github-actions

Copy link
Copy Markdown
Contributor

Visual acceptance refused: copy the exact /accept-visual <run>/<attempt> <reason> command from the report.

@freddymeta

Copy link
Copy Markdown
Contributor Author

Re-verified on the current head (main merged, 4 commits), including the Tokenizer case I had not actually measured before.

Tokenizer is the stronger proof, because its lane genuinely changes width — the story carries a token, endContent ("1 selected") and a clear button, and the spinner joins them mid-search:

state lane width input padding-inline-end caret under lane
idle 101px 109px 0
searching 123px 131px 0
settled 101px 109px 0

The padding tracks a 22px swing in both directions — calc(8px inset + var(--_astryx-end-lane-width)) — with no React commit for either transition. That is the mechanism working, not just the end state being correct.

Typeahead at 280px, for completeness: busy → content box ends 268, lane starts 276, overlap 0, --_astryx-end-lane-width: 14px; settled → the property is removed and padding returns to 0.

Also confirmed, since these are easy to claim and easy to get wrong:

  • No state in the hook: 0 useState, 0 new ResizeObserver, 4 references to the shared observeResize/unobserveResize singleton.
  • 421 tests pass across Typeahead, Tokenizer, Field and Text (Text shares the observer).
  • Lint is clean. The 3 warnings on BaseTypeahead.tsx are pre-existing — identical on main at line 375; my change only shifted them to 401.
  • The hook is 30 lines of logic with cleanup via the React 19 ref-callback return, so there is no separate teardown path to keep in step.

github-actions Bot added a commit that referenced this pull request Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typeahead/Tokenizer: the busy indicator is a static clock, and it collides with the clear button

2 participants